Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugins Phase 1 #1469

Merged
merged 13 commits into from
Apr 28, 2020
Merged

Conversation

estroz
Copy link
Contributor

@estroz estroz commented Apr 9, 2020

This PR contains the implementation of concepts outlined in the CLI plugins phase 1 design doc.

Explanation of package changes/additions:

  • cmd/: remove all but edit, update, version, and completion subcommands in favor of plugins that implement removed commands. The remaining commands are specific to kubebuilder and should not be reserved command names.
  • internal/cmdutil: commandOptions -> RunOptions.
  • pkg/cli: contains the CLI interface and cobra CLI constructors. Most commands
    from cmd/ were move here and modified to initialize plugins.
  • pkg/plugin: Base and *PluginGetter interfaces for v1 and v2 scaffolds. These are kubebuilider's base Go plugins consumed by a CLI.

This branch is a master rebase of the feature branch to which all commits here were reviewed and merged. I've saved that branch's history to preserve merge commits for further review.

TODO's:

/kind feature

/cc @DirectXMan12 @mengqiy @joelanford @hasbro17 @camilamacedo86 @Adirio

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 9, 2020
if c.MultiGroup {
return fmt.Errorf("multiple group support can't be enabled for version %s", c.Version)
func (o *editOptions) Validate() error {
if !o.config.IsV2() && !o.config.IsV3() {
Copy link
Member

@camilamacedo86 camilamacedo86 Apr 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !o.config.IsV2() && !o.config.IsV3() {
if o.config.IsV1() {

Copy link
Member

@camilamacedo86 camilamacedo86 Apr 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make easier remove the if to clean the code if/when we remove V1 impl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this was the intent, but using !o.config.IsV2() && !o.config.IsV3() means that we would need to explicitly assert that a future version (e.g. v4) supports multi-group. This is probably safer than assuming that all future versions will support multi-group.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once #1355 is merged I will rebase to remove v1 checks.

cmd/update.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2020
These changes are the implementation of concepts outlined in
designs/extensible-cli-and-scaffolding-plugins-phase-1.md

* cmd/: remove all but 'edit' and 'update' subcommands in favor
of plugins that implement removed commands

* internal/cmdutil: RunOptions configure Run() to execute a command
in a generic manner

* pkg/cli: CLI interface and cobra CLI constructors. Most commands
from cmd/ were move here and modified to initialize plugins

* pkg/plugin: Plugin and PluginGetter interfaces for v1 and v2
scaffolds. These are kubebuilider's base Go plugins consumed by
pkg/cli.CLI
pkg/plugin: validation for plugin version strings

pkg/internal/validation: project-wide validation logic; move
pkg/model/resource/validation.go functions here
internal type

pkg/plugin: InjectConfig passes a Config object to a plugin that
the plugin can modify instead of having the plugin load/save the
plugin itself

pkg/cli: manage initialization, loading, and saving of a Config
object, and inject the Config object into plugins

internal/cmdutil: remove LoadConfig and Config object parameters
from RunOptions interface

cmd: rewrite edit/update to respect new command runner interface
should run for a subcommand. This option is global. Refactoring of
getBaseFlags -> parseBaseFlags allows multiple global flags to be
parsed for a cli struct. plugin filtering considers short names,
ex. --plugins="go" rather than --plugins="go.kubebuilder.io/v2.0.0".
WithDefaultPlugins sets the default plugins a CLi should use in case
--plugins or the config 'layout' are not set

pkg/model/config: Config support the 'layout' key which indicates the
plugin that scaffolded a project, by plugin key

pkg/plugin: SplitKey, KeyFor, Key, GetShortName are utility functions
for plugin keys

* pkg/{cli,plugin}: add plugin name validation

* pkg/plugin/{v1,v2}: set layout key in config

 Please enter the commit message for your changes. Lines starting
'plugins' key, with each plugin's config keyed by its plugin key,
and an encoder and decoder for plugin configuration objects. Add
marshal/unmarshal functions for a Config

internal/config: use Config marshalling/unmarshalling to read/write
a config, and add tests
… a version

bump. This new version will be '3-alpha', which will eventually be bumped to
'3' once the below config changes have stabilized.

*: add tests and checks for v3 where appropriate
testdata: add v3 test data
the correct command name

pkg/cli: use layout or cliPluginKeys in error messages if no plugin
for a command is found
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2020
@estroz estroz marked this pull request as ready for review April 15, 2020 19:01
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 15, 2020
@estroz
Copy link
Contributor Author

estroz commented Apr 15, 2020

/hold

so this isn't merged before 2+ reviews happen.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2020
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shows be missing :

  • rebase with the branch
  • rebase with master
  • run make generate

@estroz
Copy link
Contributor Author

estroz commented Apr 18, 2020

/test pull-kubebuilder-e2e-k8s-1-14-1

go.mod Outdated Show resolved Hide resolved
pkg/cli/api.go Outdated Show resolved Hide resolved
// Set base flags that require pre-parsing to initialize c.
fs.BoolVarP(&help, helpFlag, "h", false, "print help")
fs.StringVar(&c.projectVersion, projectVersionFlag, c.defaultProjectVersion, "project version")
fs.StringVar(&c.cliPluginKey, pluginsFlag, "", "plugins to run")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this plural for forward-compatibility reasons? Should this use fs.StringSliceVar()?

For now, should we check to ensure only 1 plugin is provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For forward compatibility reasons yes. I have it as a string so users aren't confused about what type (string vs string list) they can pass to --plugins. Once phase 2 is done we can update the flag type without actually breaking the flag API.

pkg/cli/cli.go Outdated Show resolved Hide resolved
pkg/cli/cli.go Outdated Show resolved Hide resolved
pkg/cli/cli.go Outdated Show resolved Hide resolved
if c.MultiGroup {
return fmt.Errorf("multiple group support can't be enabled for version %s", c.Version)
func (o *editOptions) Validate() error {
if !o.config.IsV2() && !o.config.IsV3() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this was the intent, but using !o.config.IsV2() && !o.config.IsV3() means that we would need to explicitly assert that a future version (e.g. v4) supports multi-group. This is probably safer than assuming that all future versions will support multi-group.

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't made it all the way through, but I wanted to give you what I have so far. Looks great overall!

internal/config/config_test.go Outdated Show resolved Hide resolved
pkg/cli/api.go Outdated Show resolved Hide resolved
pkg/cli/cli_test.go Show resolved Hide resolved
pkg/cli/init.go Outdated Show resolved Hide resolved
pkg/model/config/config.go Outdated Show resolved Hide resolved
)

// Scaffolding versions
const (
Version1 = "1"
Version2 = "2"
Version3 = "3-alpha"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when we have 3-beta and 3? Should this const be called Version3Alpha?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably

Copy link
Contributor Author

@estroz estroz Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And go through a deprecation process from 3-alpha and 3-beta following a few releases of 3, or remove them once 3 is out?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we don't break anything from 3-alpha to 3-beta to 3, perhaps we can auto-promote the projectVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I was thinking. That jives with IsV3() instead of direct config.Version == VersionN comparison too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

pkg/model/config/config.go Outdated Show resolved Hide resolved
pkg/model/config/config_test.go Outdated Show resolved Hide resolved
test/e2e/v3/e2e_suite.go Outdated Show resolved Hide resolved
pkg/cli/webhook.go Outdated Show resolved Hide resolved
Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly nits. Many should be considered nonblocking.

pkg/cli/init.go Outdated Show resolved Hide resolved
pkg/model/config/config.go Outdated Show resolved Hide resolved
pkg/model/config/config.go Show resolved Hide resolved
pkg/model/config/config.go Outdated Show resolved Hide resolved
}

// Key returns a unique identifying string for a plugin's name and version.
func Key(name, version string) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: just do type Key string and make some of the split, etc be methods on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followup?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@estroz
Copy link
Contributor Author

estroz commented Apr 28, 2020

Post-review updates made in #1485, commits will be cherry-picked here after merging.

@estroz
Copy link
Contributor Author

estroz commented Apr 28, 2020

/test pull-kubebuilder-e2e-k8s-1-17-0

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shows OK for me. I will set approve. Please wait until we have the green flag to merge and at least more one ok.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, estroz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2020
… "3-alpha",

and update all references

pkg/cli: generalize layout key check for future v3 project version types
@DirectXMan12
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 28, 2020
@DirectXMan12
Copy link
Contributor

/hold cancel

1 similar comment
@estroz
Copy link
Contributor Author

estroz commented Apr 28, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 28, 2020
@k8s-ci-robot k8s-ci-robot merged commit d6e2b99 into kubernetes-sigs:master Apr 28, 2020
@estroz estroz deleted the chore/rebase-plugins branch April 28, 2020 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants